[CLI][Move] support public structs/enums as txn args in CLI#18591
[CLI][Move] support public structs/enums as txn args in CLI#18591rahxephon89 wants to merge 2 commits intoteng/public-struct-txn-argsfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
88acdd2 to
dfd47a0
Compare
fc1c26d to
d31e4d0
Compare
155b20c to
398a435
Compare
f37ad1b to
b30d55a
Compare
170c1ef to
e22d876
Compare
8f27993 to
dd7a3d2
Compare
bfc10dc to
ff0ca1f
Compare
dd7a3d2 to
966306b
Compare
ff0ca1f to
12c09c2
Compare
12c09c2 to
f69d861
Compare
966306b to
a6c536f
Compare
aptos-move/move-examples/cli-e2e-tests/common/sources/cli_e2e_tests.move
Outdated
Show resolved
Hide resolved
f69d861 to
faf499b
Compare
| } | ||
|
|
||
| /// Convert MoveType to TypeTag. | ||
| fn convert_move_type_to_type_tag(move_type: &MoveType) -> CliTypedResult<TypeTag> { |
There was a problem hiding this comment.
It's worth checking if we already have APIs to do this and other convertions defined in this file.
| } | ||
|
|
||
| /// Parse a value based on its Move type and encode to BCS. | ||
| fn parse_value_by_type<'a>( |
There was a problem hiding this comment.
Can we simply make it a async fn so that we can simplify the return type?
There was a problem hiding this comment.
Also, some abstraction would help this function to be more readable and maintainable.
| ))); | ||
| } | ||
|
|
||
| match move_type { |
There was a problem hiding this comment.
Do we have existing APIs doing this thing?
| })?; | ||
| bcs::to_bytes(&v).map_err(|e| CliError::BCS("bool", e)) | ||
| }, | ||
| MoveType::U8 => { |
There was a problem hiding this comment.
The parsing here and below has an inconsistent error handling with MoveType::Bool.
There was a problem hiding this comment.
do you mean the error type is different?
| // 1. Array format (legacy): [] or [value] | ||
| // 2. Enum format (new): {"None": {}} or {"Some": {"0": value}} | ||
|
|
||
| if value.is_array() { |
There was a problem hiding this comment.
I remember to have seen similar code in several other places? Maybe making a function for it?
| } | ||
| } | ||
|
|
||
| match qualified_name.as_str() { |
There was a problem hiding this comment.
Can we have a more systematic and generic way to handle these special cases? I suppose they will grow as time goes by.
There was a problem hiding this comment.
this corresponds to allowed structs so I will keep it as it is and add TODO once we have a more general way to handle them
| let s = if value.is_string() { | ||
| value.as_str().unwrap() | ||
| } else if value.is_number() { | ||
| &value.to_string() |
There was a problem hiding this comment.
We use value.as_str().unwrap() above and &value.to_string() here. Is it intended?
|
@rahxephon89 I did a first look at the PR. The code makes sense but can benefit from some clean-up and refactoring. I will take another look afterward. |
5e897f2 to
dac7a55
Compare
a6c536f to
810bb48
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
dac7a55 to
b89306f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
b89306f to
de9d553
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
de9d553 to
a91387e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
a91387e to
ccaeb25
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
ccaeb25 to
3a1dc50
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
3a1dc50 to
498bca7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
498bca7 to
0904383
Compare
810bb48 to
fb023bd
Compare
0904383 to
1158753
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } else { | ||
| // Module not in cache, need to fetch | ||
| } | ||
| } // Release read lock |
There was a problem hiding this comment.
Cache miss causes redundant network refetch for nonexistent types
Low Severity
In verify_struct_exists, when a module IS already cached and the struct/enum is confirmed NOT to exist (via ABI or compiled module check), the code falls through past line 118 and re-fetches the entire module from the network at line 126. The struct_exists = false result from a definitive ABI/compiled check is treated the same as the "couldn't check yet" case, causing a redundant REST API call that will reach the same conclusion.



Description
This PR:
How Has This Been Tested?
Newly added cli e2e tests.
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Medium Risk
Changes CLI transaction argument parsing to perform async REST lookups and BCS encoding for complex types, which can affect
move run/view/simulatebehavior and error handling across networks.Overview
Adds CLI support for passing public Move structs and enums as transaction arguments via
--json-file, by introducing an async parsing path that fetches on-chain module bytecode/ABI and BCS-encodes nested struct/enum values (includingOption<T>in both legacy vector and new variant formats).Updates
move run,move simulate, andmove viewto use the new async argument parsing when JSON input is used, adds new errors/typing helpers (e.g.,MODULE_SEPARATORconstants), and includes a newStructArgParserwith module caching and nesting-depth limits.Expands CLI e2e coverage with a new Move package and Python tests for struct/enum args, and enhances the e2e harness with an optional
--use-local-testnetmode (auto-start/stop via local CLI) plus pins Move compile/publish language version to2.4in relevant tests.Written by Cursor Bugbot for commit 1158753. This will update automatically on new commits. Configure here.